Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add keyboard_layout stanza. #15061

Merged
merged 3 commits into from Mar 27, 2023
Merged

Conversation

reitermarkus
Copy link
Member

@reitermarkus reitermarkus commented Mar 26, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

See Homebrew/homebrew-cask#143776.

@@ -15,6 +15,7 @@ class Config

DEFAULT_DIRS = {
appdir: "/Applications",
keyboard_layoutdir: "/Library/Keyboard Layouts",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
keyboard_layoutdir: "/Library/Keyboard Layouts",
keyboard_layoutdir: "~/Library/Keyboard Layouts",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. You will need sudo to delete the cache anyways, so might as well install in the system-wide directory. Arguably, all of these should use the system-wide directory by default, but it would be pretty hard to migrate these now.

Comment on lines +31 to +33
def delete_keyboard_layout_cache(command: nil, **_)
command.run!("/bin/rm", args: ["-f", "--", "/System/Library/Caches/com.apple.IntlDataCache.le*"], sudo: true)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to take a command argument if it's always going to run the same command?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The command argument here contains more information than calling system_command! directly, e.g. whether it should be verbose, so that all commands for a given cask behave the same way in terms of output.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I looked at the other artifacts and see that this is a common pattern. I wonder why it accepts nil as an argument though since it seems to be required here and I couldn't find any references where the command would be nil in the cask installer.

Comment on lines +19 to +27
def install_phase(**options)
super(**options)
delete_keyboard_layout_cache(**options)
end

def uninstall_phase(**options)
super(**options)
delete_keyboard_layout_cache(**options)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is is always necessary to delete the layout cache on both install and uninstall? The PR linked to in the homebrew/cask repo only shows that this is done for one cask.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, https://www.kaufmann.no/roland/dvorak/macosx.html shows its removal in both installation and removal instructions.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me when it does to @apainintheneck!

@MikeMcQuaid MikeMcQuaid merged commit cf6614b into Homebrew:master Mar 27, 2023
22 checks passed
@MikeMcQuaid
Copy link
Member

Actually, looks like @apainintheneck comments addressed (please contradict me if wrong!) so merging now to hopefully get into today's release.

@reitermarkus reitermarkus deleted the keyboard-layout branch March 27, 2023 16:06
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants